Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

components/DatePicker.tsx (1)

20-26: Consider removing commented-out code.

The commented code blocks explaining the removed showPicker() functionality could be replaced with a brief inline comment. The git history preserves the original implementation if needed later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/DatePicker.tsx` around lines 20 - 26, Remove the stale
commented-out code related to the removed showPicker() API: delete the commented
declarations and function for inputRef, handleIconClick and the showPicker()
call in DatePicker.tsx, and replace them with a single concise inline comment
noting that programmatic opening via showPicker was removed; reference the
identifiers inputRef, handleIconClick, and showPicker in the comment so future
readers know what was intentionally removed.

scripts/check-db-contract.mjs (1)

81-83: Consider adding error handling for signOut failures.

If signOut() fails (e.g., network issue), any error thrown would mask the original test failure. Consider wrapping in a try-catch to log but not re-throw signOut errors.

🛡️ Proposed defensive signOut

 } finally {
-  await supabase.auth.signOut();
+  try {
+    await supabase.auth.signOut();
+  } catch (signOutError) {
+    console.warn('Failed to sign out during cleanup:', signOutError.message);
+  }
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/check-db-contract.mjs` around lines 81 - 83, The finally block
currently awaits supabase.auth.signOut() without guarding against failures,
which could mask an earlier test error; wrap the await supabase.auth.signOut()
call in a try-catch inside the existing finally, await it inside the try, and in
the catch log the sign-out error (e.g., console.error or existing logger) but do
not re-throw so the original test error is preserved; specifically modify the
finally that calls supabase.auth.signOut() to a try { await
supabase.auth.signOut(); } catch (err) { /* log sign-out failure with context */
}.

tests/e2e/02-section-settings-workflow.md (1)

17-23: Add an explicit post-cleanup persistence check.

After restoring the original value, include a save + reload confirmation step so cleanup guarantees are verifiable, not implied.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/02-section-settings-workflow.md` around lines 17 - 23, Update the
"Restore the original meeting day value before leaving the page." step in
tests/e2e/02-section-settings-workflow.md to explicitly persist and verify the
cleanup: after resetting the meeting day back to the original value perform the
save action (e.g., trigger the test's save/submit flow and wait for any success
indicator), then reload the page (or navigate away and back) and assert the
meeting day equals the original value; ensure the new assertions replace the
current implied cleanup so the test fails if the restore does not actually
persist.

docs/04-deployment.md (1)

34-37: Optional docs polish: reduce repeated “Verify” openings in checklist items.

This is readable as-is, but varying the leading verb in adjacent steps would make the checklist flow smoother.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/04-deployment.md` around lines 34 - 37, The checklist repeats the verb
"Verify" for consecutive items (the lines starting "Verify sign-in works for
manually provisioned users.", "Verify the production users have the expected
roles in `profiles`.", "Verify `settings` has seeded rows for both `company` and
`junior`.", and "Smoke-test auth, section settings persistence, member CRUD,
marks entry, and dashboard behavior against the live backend."); update these
items to use varied leading verbs (e.g., "Confirm sign-in works...", "Ensure
production users have the expected roles in `profiles`.", "Check `settings`
contains seeded rows for both `company` and `junior`.", and keep or slightly
reword the final "Smoke-test..." line) so the checklist reads more smoothly
while preserving the same checks and scope.

.github/workflows/ci-infrastructure.yml (1)

33-60: Consider gating live-Supabase smoke steps so core CI still runs without secrets.

Right now, missing live secrets fail the job before typecheck/unit/build. If fork PRs or secretless validation are expected, conditionally run the live contract/e2e steps and keep static/unit checks unconditional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci-infrastructure.yml around lines 33 - 60, Make the
live-Supabase steps conditional so forks and secretless CI still run static
checks: add an if expression that requires all four secrets to be present to
each of the live steps ("Check required Supabase client and smoke-test env
vars", "Smoke-test Supabase client startup and runtime config", "Run
client-visible database contract smoke check", "Install Playwright browser
dependencies", and "Run browser smoke tests against live Supabase data"); e.g.
use if: ${{ secrets.VITE_SUPABASE_URL != '' && secrets.VITE_SUPABASE_ANON_KEY !=
'' && secrets.E2E_TEST_EMAIL != '' && secrets.E2E_TEST_PASSWORD != '' }} so the
remaining steps (typecheck, test:run, build) run unconditionally.

services/db.test.ts (1)

209-255: Tighten the members write-path assertions in these tests.

These cases currently prove routing (members table, no RPC) but not the actual write parameters. They would still pass if updateBoy() skipped update(...), or if either method used the wrong eq(...) filters.

🔍 Tighten the assertion surface

   return {
     from,
     rpc,
+    memberUpdate,
+    memberUpdateEqId,
+    memberUpdateEqSection,
+    memberDeleteEqId,
+    memberDeleteEqSection,
     get rpcResponse() {

     expect(supabaseMock.from).toHaveBeenCalledWith('members');
+    expect(supabaseMock.memberUpdate).toHaveBeenCalledWith({
+      name: 'Alex Updated',
+      school_year: '11',
+      section: 'company',
+      squad: 3,
+      is_squad_leader: true,
+    });
+    expect(supabaseMock.memberUpdateEqId).toHaveBeenCalledWith('id', 'member-1');
+    expect(supabaseMock.memberUpdateEqSection).toHaveBeenCalledWith('section', 'company');
     expect(supabaseMock.rpc).not.toHaveBeenCalled();
     expect(supabaseMock.from.mock.calls.filter(([table]) => table === 'marks')).toHaveLength(1);

     expect(supabaseMock.from).toHaveBeenCalledTimes(1);
     expect(supabaseMock.from).toHaveBeenCalledWith('members');
+    expect(supabaseMock.memberDeleteEqId).toHaveBeenCalledWith('id', 'member-1');
+    expect(supabaseMock.memberDeleteEqSection).toHaveBeenCalledWith('section', 'junior');
     expect(supabaseMock.rpc).not.toHaveBeenCalled();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@services/db.test.ts` around lines 209 - 255, The tests currently only assert
routing to 'members' and absence of RPC but not the exact write arguments;
update the two tests to assert the concrete write parameters: for the updateBoy
test, verify that supabaseMock.from('members').update was called once with the
expected partial payload (only allowed member fields — e.g.,
name/squad/year/is_squad_leader mapping from the input) and that the returned
query used eq('id', 'member-1') and eq('section', 'company'); for the
deleteBoyById test, verify that supabaseMock.from('members').delete was called
with no payload (or called once) and that its query used eq('id', 'member-1')
and eq('section', 'junior'); keep the existing checks that rpc was not called.
Ensure you reference the chain starting from supabaseMock.from so you assert
.update/.delete call arguments and subsequent .eq filters used by updateBoy and
deleteBoyById.

components/weeklyMarksSavePlan.ts (1)

67-67: Type assertion could be made safer.

The cast (markState as JuniorMarkState | undefined) assumes the caller provides the correct type for junior section. If a company-style number is accidentally passed, accessing .uniform and .behaviour will return undefined, which is handled by the fallback, but consider adding a runtime type guard for defense-in-depth.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/weeklyMarksSavePlan.ts` at line 67, The current cast of markState
to JuniorMarkState when creating juniorState is unsafe; add a runtime type guard
(e.g., isJuniorMarkState) that checks markState is an object and has string
properties "uniform" and "behaviour", then use that guard instead of a blind
cast when computing juniorState so you only read .uniform and .behaviour when
safe, falling back to { uniform: '', behaviour: '' } otherwise; update the code
that references juniorState to rely on this guarded assignment.

services/db.ts (2)

13-18: Duplicate type definition.

WeeklyMarksSnapshotEntry is also defined in components/weeklyMarksSavePlan.ts (line 7). Consider importing from a single source to avoid drift.

♻️ Suggested consolidation

Either:

1. Export from weeklyMarksSavePlan.ts and import here, or
2. Move shared types to a dedicated types file (e.g., types.ts or a new types/marks.ts)

-export type WeeklyMarksSnapshotEntry = {
-  memberId: string;
-  mark: Mark | null;
-};
+import { WeeklyMarksSnapshotEntry } from '../components/weeklyMarksSavePlan';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@services/db.ts` around lines 13 - 18, Duplicate type
WeeklyMarksSnapshotEntry/WeeklyMarksSaveEntry exists; remove the local
definition and import the canonical WeeklyMarksSnapshotEntry from the existing
declaration (the one in components/weeklyMarksSavePlan.ts) or move both to a
shared types module (e.g., types/marks.ts) and import from there, then re-export
WeeklyMarksSaveEntry as an alias of the imported WeeklyMarksSnapshotEntry to
preserve existing exports; update any imports that referenced the removed local
type to use the single exported symbol.

---

121-129: Consider simplifying the mark transformation.

The IIFE pattern for extracting and removing date is functional but adds cognitive overhead. A helper function or destructuring at the call site could improve readability.

♻️ Suggested simplification

     p_snapshot: snapshot.map(({ memberId, mark }) => ({
       memberId,
-      mark: mark
-        ? (() => {
-            const { date: _date, ...storedMark } = toStoredMark(mark, section);
-            return storedMark;
-          })()
-        : null,
+      mark: mark ? toStoredMarkWithoutDate(mark, section) : null,
     })),

Where toStoredMarkWithoutDate is a simple helper:

const toStoredMarkWithoutDate = (mark: Mark, section: Section) => {
  const { date: _date, ...rest } = toStoredMark(mark, section);
  return rest;
};

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@services/db.ts` around lines 121 - 129, The p_snapshot mapping uses an IIFE
to call toStoredMark(mark, section) and strip its date field which is hard to
read; create a small helper (e.g., toStoredMarkWithoutDate(mark, section)) that
calls toStoredMark, destructures out date and returns the rest, then replace the
IIFE in the p_snapshot map with a call to that helper for clarity and reuse;
reference p_snapshot, toStoredMark, mark, and section when making the change.

components/weeklyMarksSavePlan.test.ts (2)

124-139: Consider expanding test coverage for normalizeEditableMarksForSave.

The current tests cover company marks and absent junior marks, but the implementation has additional branches (junior section with valid uniform/behaviour scores, empty string → 0 conversion). Additional test cases would strengthen confidence in the normalization logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/weeklyMarksSavePlan.test.ts` around lines 124 - 139, Add tests
inside the existing describe('normalizeEditableMarksForSave') suite to cover the
junior-mark branches: one test where a junior mark includes numeric uniformScore
and behaviourScore (e.g., uniformScore: '2', behaviourScore: '3') and assert
those are preserved and converted to numbers, and another test where
uniformScore/behaviourScore are empty strings and should be normalized to 0
(instead of ''). Use the function normalizeEditableMarksForSave and mirror the
existing test style to assert expected normalized output for both cases.

---

141-156: Consider adding edge case tests for areMarkListsEqual.

Currently only tests order-insensitivity. Consider adding tests for: differing list lengths (should return false), marks with same date but different scores, and junior marks with uniformScore/behaviourScore fields.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/weeklyMarksSavePlan.test.ts` around lines 141 - 156, Add edge-case
tests for areMarkListsEqual: add a test asserting false for differing list
lengths, a test asserting false for two marks with the same date but different
score values (e.g., score 5 vs 6), and a test covering junior-mark objects that
use uniformScore/behaviourScore fields (both equal should return true, differing
should return false). Place these new it() blocks alongside the existing
order-insensitivity test and reference the areMarkListsEqual function in each
assertion.

components/WeeklyMarksPage.tsx (1)

203-219: ⚠️ Potential issue | 🟡 Minor

isDirty is still sticky after reverting to the original snapshot.

The no-op save path clears the flag only after the user clicks Save. If they edit back to the original values and then change weeks, requestDateChange() still sees isDirty === true and opens the discard modal even though buildWeeklyMarksSnapshot() is empty. Use the snapshot itself to drive both the modal gate and the Save FAB.

Also applies to: 257-268

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/WeeklyMarksPage.tsx` around lines 203 - 219, requestDateChange
currently relies on the boolean isDirty which stays true even if the user
reverts edits; change the gating logic to compute the snapshot via
buildWeeklyMarksSnapshot() and use its emptiness instead of isDirty when calling
shouldConfirmWeeklyMarksDateChange and when rendering/enabling the Save FAB.
Specifically, update requestDateChange to pass the computed snapshot (or a
boolean derived from it) into shouldConfirmWeeklyMarksDateChange and only call
setPendingDate/setIsDateChangeConfirmOpen when the snapshot indicates real
changes; likewise, drive the Save FAB enabled/visible state from
buildWeeklyMarksSnapshot() emptiness rather than the isDirty flag so reverting
edits clears the modal gate and save affordance.

tests/e2e/smoke.e2e.ts (1)

128-142: Consider guarding openSectionSettings against already-open state.

If the main test body completes successfully, Section Settings will already be open when the finally block runs. Calling openSectionSettings(page) again at line 130 clicks the button, which could toggle the modal closed if the UI treats it as a toggle, potentially causing the subsequent assertions to fail spuriously.

If the UI button always opens (never toggles), this is fine. Otherwise, consider checking whether the heading is already visible before clicking:

🛡️ Optional guard

     } finally {
       try {
+        const settingsHeading = page.getByRole('heading', { name: 'Section Settings' });
+        if (!(await settingsHeading.isVisible())) {
+          await openSectionSettings(page);
+        }
-        await openSectionSettings(page);
         const restoreSelect = page.getByLabel('Weekly Meeting Day');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/smoke.e2e.ts` around lines 128 - 142, The finally block can call
openSectionSettings(page) even when the Section Settings modal is already open,
which may toggle it closed; update the cleanup to first check whether the
settings modal is already visible (e.g., query for the Section Settings heading
or dialog role) and only call openSectionSettings(page) when it is not visible,
then proceed to read/restore Weekly Meeting Day via restoreSelect and
saveSettingsAndWait; reference openSectionSettings(page), the restoreSelect
(page.getByLabel('Weekly Meeting Day')), saveSettingsAndWait, and restoreError
to keep existing error capture.

components/WeeklyMarksPage.tsx (1)

45-45: Remove dead isDirty state.

The isDirty state value is never read—hasPendingChanges derived from the snapshot is used everywhere instead. The setIsDirty calls (lines 104, 163, 195, 214, 274, 285) cause unnecessary re-renders without affecting behavior. Consider removing this state entirely along with all setIsDirty calls.

♻️ Remove dead state and its setter calls

-  const [, setIsDirty] = useState(false); // Retained for local input change tracking; navigation gating is derived from the snapshot.

Then remove all setIsDirty(...) calls throughout the file (lines 104, 163, 195, 214, 274, 285).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/WeeklyMarksPage.tsx` at line 45, Remove the dead isDirty state and
its setter to avoid unnecessary renders: delete the useState declaration "const
[, setIsDirty] = useState(false);" in the WeeklyMarksPage component and remove
every call to setIsDirty(...) inside that component (these calls occur in
handlers that already rely on hasPendingChanges derived from the snapshot).
Ensure no other code references isDirty or setIsDirty afterwards; if there are,
replace those references to use hasPendingChanges or the appropriate
snapshot-derived value instead.

components/WeeklyMarksPage.tsx (1)

288-288: today can become stale if the page stays open past midnight.

The empty dependency array means today is computed once on mount. If a user keeps the page open across midnight, isPastDate (line 381) will use a stale value. The isLocked effect (lines 66-70) also won't re-run since it only depends on selectedDate.

This is a low-probability edge case, but for consistency you could derive today inline or refresh it on visibility change.

♻️ Optional: derive inline or refresh on focus

- const today = useMemo(() => new Date().toISOString().split('T')[0], []);
+ const today = new Date().toISOString().split('T')[0];

Or, if you want to avoid recomputing on every render, consider refreshing when the document regains focus.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@components/WeeklyMarksPage.tsx` at line 288, The computed `today` value is
created once with useMemo and can become stale across midnight; change its usage
so it is computed fresh when needed (e.g., derive `today` inline instead of via
`useMemo`) or update it on visibility/focus so dependent logic like `isPastDate`
and the `isLocked` effect that depends on `selectedDate` re-evaluates;
specifically, replace the `const today = useMemo(...)` pattern in
WeeklyMarksPage with either an inline `new Date().toISOString().split('T')[0]`
where `today` is used (so `isPastDate` always sees current date) or add a small
effect that updates `today` on `visibilitychange`/`focus` and use that state
variable so `isPastDate` and the `isLocked` effect re-run when the day changes.

tests/e2e/smoke.e2e.ts (1)

126-141: Consider adding an ensureWeeklyMarksPageOpen helper for consistency.

The section settings test uses ensureSectionSettingsOpen for idempotent navigation in the restore path, but the weekly marks restore unconditionally calls openWeeklyMarksPage. While this works (since clicking the nav button is likely idempotent), an ensure* variant would be more explicit and consistent with the settings pattern.

♻️ Optional consistency improvement

+const ensureWeeklyMarksPageOpen = async (page: Page) => {
+  const heading = page.getByRole('heading', { name: 'Weekly Marks' });
+
+  if (!(await heading.isVisible())) {
+    await openWeeklyMarksPage(page);
+  }
+};

Then in the restore block:

       try {
-        await openWeeklyMarksPage(page);
+        await ensureWeeklyMarksPageOpen(page);
         await dateInput.fill(marksDate);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/e2e/smoke.e2e.ts` around lines 126 - 141, Add an idempotent navigation
helper ensureWeeklyMarksPageOpen and use it in the restore/finally path instead
of calling openWeeklyMarksPage unconditionally; implement
ensureWeeklyMarksPageOpen to detect whether the weekly marks view is already
active (or verify by selector used in openWeeklyMarksPage) and only click the
nav button when needed, then update the finally block to call
ensureWeeklyMarksPageOpen(page) before interacting with dateInput,
restoreScoreInput and saveWeeklyMarksAndWait to mirror the
ensureSectionSettingsOpen pattern.